Fix build with GCC 15 #56243
Fix build with GCC 15 #56243
Conversation
Fixes ray-project#56242 Signed-off-by: Iyán Méndez Veiga <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a build failure with GCC 15 by including the <cstdint> header. This is a valid workaround for an upstream dependency issue. The change is correct, but to improve maintainability and prevent future issues with code formatters, I've suggested adding a comment to explain the non-standard include order.
|
|
||
| #pragma once | ||
|
|
||
| #include <cstdint> |
There was a problem hiding this comment.
This change correctly fixes the build with GCC 15. However, since the placement of <cstdint> is a workaround and intentionally violates the project's standard include order (as you noted regarding format.sh), it's crucial to add a comment. This will prevent other developers from 'fixing' the order in the future and reintroducing the build failure. The comment should explain why this specific order is necessary and refer to the relevant issues.
| #include <cstdint> | |
| // Included before opentelemetry headers to fix build with GCC 15. This is a | |
| // workaround for a missing include in opentelemetry-cpp, see | |
| // https://github.com/open-telemetry/opentelemetry-cpp/pull/3240. | |
| // See Ray issue #56242. Do not re-order. | |
| #include <cstdint> |
|
Should we just upgrade opentelemetry-cpp? cc @can-anyscale |
|
Also currently we don't officially support building using GCC 15. |
+1, Can you try building with something around gcc 9-11 or clang 12 is the official one we recommend. This will make us turn the clang format off for this section and also will be removed by iwyu if we turn it on in the future. |
I really don't want to use unsupported toolchains. In Arch Linux we have gcc 14-15 and clang 18-20 in the official repos. Why are you only supporting such old versions? |
It's difficult to upgrade the manylinux version we use to build the Python wheels cc @aslonnie. We are working on being better at supporting newer versions. There's some dependency upgrades we need to get through and then should be able to upgrade open telemetry too. You might have better luck with clang 18, I use clang 17 to build on my machine and it works. |
|
Okay, then closing this since gcc 15 is not supported. |
we need to keep building on manylinux2014 for compatibility with, well, many linux systems. |
You mean keep building on an image based on CentOS 7 which is EOL since more than a year ago? Are you aware of users running ray on such old Linux systems? |
it is a standard on libc and dynamic library symbol sets. it just happens to be based on centos, but the lifecycle is really independent of centos. for example conda / conda-forge also maintains compatibility with the standard so that python packages and other native dynamic libraries can work well together. in theory, as long as the resulting wheel satisfies the standard, it does not have to compile inside the manylinux2014 standard container image (which pins the c++ compiler version), but the task is believed to be very challenging (if not practically impossible) to port the toolchain out of it the container and perform cross-compiling effectively (which will allow people to use a different compiler version) |
|
one option is that the change can be guarded with macros that is disabled on manylinux2014, and preferably also have CI tests with the macros enabled. |
|
ultimately ray core team's decision though. |
Then why not keep using manylinux2010? Or manylinux1? Even more supported systems, right? Already the first paragraph of PEP 599 is very clear about this:
Exactly the same argument can be made against keep building wheels with manylinux2014. But yeah, as you said, this is up to ray core team. My point is that I don't think moving to a newer manylinux image, and supporting newer toolchains will impact users of ray. |
:) if we can we actually will try to keep using them.. but the toolchain images stop updating and the package source for those images stopped working anymore. |
4 participants
Why are these changes needed?
Currently building ray from source fails when using GCC 15.
Ideally, this should be fixed in opentelemetry-cpp like they did here (or perhaps it's already fixed in newer versions?) but this workaround is just an extra include here and allows to build on systems with newer versions of GCC.
Related issue number
Fixes #56242
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.That script would move the include after all the
opentelemetryincludes, which is precisely what we don't want here.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.